Skip to content

Conversation

@ricardoV94
Copy link
Member

@ricardoV94 ricardoV94 commented Mar 18, 2022

Aeppl allows for graphs containing random variables. PyMC models do not generally allow for this, with the current exception of models that include SimulatorRVs.

This PR adds a check to avoid subtle bugs when RandomVariables creep in into the logp unexpectedly, which can happen when mis-using DensityDist or Interval transforms.

Closes #5155

@codecov
Copy link

codecov bot commented Mar 18, 2022

Codecov Report

Merging #5614 (d4880a8) into main (b895e40) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #5614   +/-   ##
=======================================
  Coverage   87.63%   87.64%           
=======================================
  Files          76       76           
  Lines       13717    13722    +5     
=======================================
+ Hits        12021    12026    +5     
  Misses       1696     1696           
Impacted Files Coverage Δ
pymc/distributions/logprob.py 96.33% <100.00%> (+0.17%) ⬆️

Comment on lines 201 to +210
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a perfect illustration of the problem that's detected by this PR

Aeppl allows for graphs containing random variables. PyMC models do not generally allow for this, with the current exception of models that include SimulatorRVs.
@ricardoV94
Copy link
Member Author

Failing test seems unrelated

@ricardoV94 ricardoV94 merged commit e77e238 into pymc-devs:main Mar 21, 2022
@ricardoV94 ricardoV94 deleted the logp_rv_check branch June 6, 2023 03:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Invalid logp expression if args aren't explicit in DensityDist

2 participants